Repository navigation
fix(storage-cassandra, #310): return proper 400 when a secondary index key is set to an empty value - #417
fix(storage-cassandra, #310): return proper 400 when a secondary index key is set to an empty value#417jcshepherd wants to merge 2 commits into
Conversation
…is set to an empty value (fixes #410) PutItem properly validated that index key attributes were non-empty, but UpdateItem and transactional Update did not. Added a utility to validate, along with a limited pytest. The pytest will overlap the tests included in PR399 (item size): can be cleaned up later if desired.
…es and non-empty index key values
|
Thanks for picking this up, and sorry for the collision. I opened #416 for the same issue the day before this one went up, so we have two fixes for #410. #416 makes the same two changes you have here on Cassandra and MongoDB, and it also fixes SQLite and PostgreSQL, which is where the CI failures on this PR come from (the SQLite job's I measured the transacted case on the service while writing #416. For an empty index key on a transacted Update it returns a top-level Would you be up for reviewing #416 instead? You've been through this code path, so you're the right person for it. I'll close this one once #416 merges, unless you'd rather close it now. |
LeeroyHannigan
left a comment
LeeroyHannigan
left a comment
There was a problem hiding this comment.
fmt and clippy are clean, the Cassandra direct tests pass (9 index, 14 put_get_item, 23 transact), and a scratch merge with main compiles. The two message constants match my service measurement, and the Cassandra standalone path refuses the empty key with that message. On the service the transacted case answers a top-level ValidationException with no CancellationReasons, so the assertion here and the Cassandra mapping to a cancellation reason are both wrong, and that assertion fails the SQLite and PostgreSQL jobs. PostgreSQL's standalone update runs no index-key check on main and this PR adds none, so both standalone cases fail there with DID NOT RAISE.
| # cancellation reason, matching real DynamoDB (see | ||
| # tests/test_transaction_key_size_validation.py for the same pattern). | ||
| resp = exc_info.value.response | ||
| assert resp["Error"]["Code"] == "TransactionCanceledException" |
There was a problem hiding this comment.
Blocking: the service answers a top-level ValidationException here, with the update expression wording and no CancellationReasons. Measured on a table with a string-keyed GSI: TransactWriteItems carrying one Update member that sets the index key to an empty string returned 400 ValidationException with CancellationReasons absent, and a two-member transaction with a passing Update beside it returned the same. This assertion is why the SQLite and PostgreSQL jobs are red (both report ValidationException), so please assert the top-level ValidationException and its message instead.
| &indexes, | ||
| &key_info.attribute_definitions, | ||
| ) | ||
| .map_err(|e| CancellationReason::validation_error(e.to_string()))?; |
There was a problem hiding this comment.
Blocking: mapping the empty-key failure to a cancellation reason makes the transacted Update answer TransactionCanceledException, which is not what the service does for this fault. A wrong index-key type does belong in a cancellation reason, and the service confirms the split: a transacted Update with a type mismatch comes back as TransactionCanceledException with a ValidationError reason, while an empty key comes back as a top-level ValidationException. Please keep the type check here and surface the emptiness failure as a top-level validation error before the members are evaluated.
| class TestEmptyIndexKeyOnUpdate: | ||
| """UpdateItem setting a GSI key to an empty value is refused on every backend.""" | ||
|
|
||
| def test_standalone_update_refuses_empty_binary_index_key(self, dynamodb_client, gsi_table): |
There was a problem hiding this comment.
Blocking: the PostgreSQL standalone UpdateItem path runs no index-key check on main (crates/storage-postgres/src/data/update_item.rs has no validate_index_key_* call) and this PR does not add one, so this case and the empty-string one next to it fail in the run-integration job with DID NOT RAISE. Either add the PostgreSQL check alongside the Cassandra one, or hold these two cases back until that path validates.
| // the UpdateExpression message context (no IndexName/ | ||
| // IndexKey suffix) matches the service and the other | ||
| // backends (issue #410). | ||
| self.validate_index_keys_for_item( |
There was a problem hiding this comment.
Minor: this call site is in update_item_impl, the standalone UpdateItem path, while the comment above reads as though it were the transacted one. MongoDB's transacted Update still passes the Item context through validate_index_keys (data_engine.rs around 2428 and 2699), so that path keeps the PutItem wording after this change.
PutItem properly validated that index key attributes were non-empty, but UpdateItem and transactional Update did not. Added a utility to validate, along with a limited pytest. The pytest will overlap the tests included in PR399 (item size): can be cleaned up later if desired.
What
Fixes the bug reported in #410 where Cassandra UpdateItem and transactional Update failed to validate the updated index key attributes were non-empty, causing a 500 to be returned on GSI update, rather than an expected 400.
Why
Conformance issue. Additionally, the transactional update failure was silent, so not only was the update incomplete, but the client was left unaware of the problem.
Closes #410
Testing done
Full pre-push suite: fmt clippy licenses unit pytest rust-tests, all integ tests run against Cassandra 4.11
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
Breaking changes
Affected APIs previously returned (incorrectly) 500s when an update resulted in empty index key values. These methods now return (correct) 400s: breaking, but restores conformance with Amazon DynamoDB.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.